Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(fee-breakdown): Initial setup and logic with useFeeBreakdown() hook #4010

Merged
merged 11 commits into from
Dec 5, 2024

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Nov 27, 2024

What does this PR do?

Sets up basic logic for parsing fee details from the passport, and displaying them to the user via the FeeBreakdown component

image

Design decisions

These are largely points up for discussion, I just want to explain some of my reasoning and outline potential next steps.

If we cannot parse values from the passport, do not show the breakdown to the user
Even if the showFeeBreakdown prop is set to true, the breakdown still won't show if we cannot display it to the user (e.g. missing or invalid values). If this happens it's a content issue which will be flagged via Airbrake for content editors to ultimately fix - the user journey won't be disrupted.

Hardcode application.fee data field
This is a rough MVP decision just to move forward initially, it's not a big additional task to read data.fn from the relevant Pay component here.

Reductions and exemptions are combined to a generic "reduction" value
The fee calculator doesn't associate a numerical value with exemptions and reductions, but just updates application.fee.payable and sets true/false values. There's a few options around how we could handle this -

  • Update content to always set a passport value, and then remove it from application.fee.payable
  • List exemptions and reduction, and sum the totals
    I'm very keen to avoid repeating any logic already handled in the fee-calculator flow - doing so puts us in an awkward code vs. low-code spot with regards to how this logic is handled.

Again, I think we take a fallback approach here. If we cannot parse a list of exemptions or reductions from the passport, we can fallback to summed generic "reductions" amount?

VAT is set via a Calculate component, and not re-calculated
I'm not sure about this one - it allows room for error if an Editor incorrectly calculates this value. A more robust approach might be to allow the user to set a application.fee.payable.includesVAT variable, and display an amount based on this.

Nothing is really explained to Editors
I think this will need something - I think just adding a list of variables and how this words in the Editor would achieve this.

Disabled for Invite to pay journeys
This is something I hadn't initially considered - we have no passport when returning to the service via invite to pay so the current approach does not work. We'll need to persist the fee breakdown data to the payment_requests table in a similar way to how govpay_metadata is stored. I'll open another PR shortly to provide a solution here.

Next steps...

Here's what I'd like to go for as I suspect the current implementation is too MVP -

  • List reductions and exemptions, but with a single summed value each (exemptions = full amount, reductions = calculated - payable)
  • Handle invite to pay journeys
  • Add explanation / documentation within the Pay editor modal if showFeeBreakdown is checked

Copy link

github-actions bot commented Nov 27, 2024

Removed vultr server and associated DNS entries

@DafyddLlyr DafyddLlyr force-pushed the dp/simple-fee-breakdown branch from ecc119b to 595d8dd Compare November 27, 2024 12:03
Comment on lines 21 to 22
applicationFee:
data["application.fee.calculated"] || data["application.fee.payable"],
Copy link
Contributor Author

@DafyddLlyr DafyddLlyr Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 35 to 41
const schema = z
.object({
"application.fee.calculated": feeSchema.optional().default(0),
"application.fee.payable": feeSchema,
"application.fee.payable.vat": feeSchema.optional().default(0),
})
.transform(toFeeBreakdown);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parse & transform pattern might be a nice approach to try in planx-core passport → payload mappings.

@DafyddLlyr DafyddLlyr marked this pull request as ready for review December 2, 2024 09:14
@DafyddLlyr DafyddLlyr force-pushed the dp/simple-fee-breakdown branch 2 times, most recently from ec21eb7 to 5569c90 Compare December 5, 2024 08:46
@DafyddLlyr DafyddLlyr marked this pull request as draft December 5, 2024 08:51
@DafyddLlyr DafyddLlyr force-pushed the dp/simple-fee-breakdown branch from 5569c90 to bd6ad07 Compare December 5, 2024 08:54
@DafyddLlyr DafyddLlyr requested a review from a team December 5, 2024 09:05
@DafyddLlyr DafyddLlyr marked this pull request as ready for review December 5, 2024 09:05
@DafyddLlyr DafyddLlyr changed the title feat: Fee breakdown logic feat(fee-breakdown): Initial setup and logic with useFeeBreakdown() hook Dec 5, 2024
Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks overall good - a few clarifying questions and language suggestions but nothing stopping merge at this point as this is all feature-flagged 👍

@@ -0,0 +1,12 @@
export interface FeeBreakdown {
applicationFee: number;
total: number;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: introducing applicationFee and total language feels like a bit of extra mental load here and I wonder if this can re-use calculated (which if undefined, fallsback to payable) and payable instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before reading the tests, I would have also personally guessed that total = full calculated amount and applicationFee = payable but your passport mappings are the other way around !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful feedback thanks - this was an intentional decision to try and make it a little clearer and more explicit (I did also consider gross/net as well at one point).

Clearly not successful though 😂 Your point about consistency and mental load is a very valid one 👍

@DafyddLlyr
Copy link
Contributor Author

DafyddLlyr commented Dec 5, 2024

Thanks very much for the review 👍

Due to branches and other PRs which rely on this I'm going to fix these issues forward - all currently feature flagged.

@DafyddLlyr DafyddLlyr merged commit 8b284ed into main Dec 5, 2024
12 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/simple-fee-breakdown branch December 5, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants